Skipping over groovy metadata class + groovy test. #118
Conversation
jr-objects/src/test/java/com/fasterxml/jackson/jr/GroovyTest.groovy
Outdated
Show resolved
Hide resolved
|
Ok, so, this would indeed avoid cyclic dependency but I have 2 concerns. One is that it would prevent any all types in Second: jackson-databind prevents serialization of all instances accessed via |
jr-objects/src/main/java/com/fasterxml/jackson/jr/ob/impl/BeanPropertyIntrospector.java
Outdated
Show resolved
Hide resolved
|
Ok, I am not sure I like this big code clean up, as part of changes. I am all for targeted pieces but wholesale cleanup seems unnecessary. |
|
Okay Will create keep this PR for just precise changes, and then create a seperate one for Code Cleanup. if its fine. |
|
Done! 👍👍 |
|
|
||
| void testSimpleObject() throws Exception { | ||
| var data = JSON.std.asString(new MyClass()) | ||
| var expected = """{"AAAAA_A_Field_Starting_With_Two_Capital_Letters":"XYZ","aDouble":0.0,"aPublicInitializedInteger":56,"aPublicInitializedIntegerObject":1516,"aPublicUninitializedInteger":0,"anInitializedIntegerObject":1112,"anInitializedPublicString":"stringData","anInitializedString":"ABC","anInteger":0,"anIntegerWithValue":12}""" |
There was a problem hiding this comment.
Was about to ask about var and triple quotes wrt JDK 8 before realizing this is Groovy :)
There was a problem hiding this comment.
One question tho: how does this test get run? It doesn't look like it runs with ./mvnw clean test from main level...
And commenting out handling of Groovy metadata makes nothing fail (ditto for explicit fail() in test).
Maybe test would need to be under src/test/groovy instead of src/test/java?
There was a problem hiding this comment.
As per
that (moving test) is one of 3 things needed, fwtw.
| * either removing that reference, or skipping over such references. | ||
| */ | ||
| protected static boolean isGroovyMetaClass(Class<?> clazz) { | ||
| return clazz.getName().startsWith("groovy.lang"); |
There was a problem hiding this comment.
This needs to check for class name, not just package, right?
There was a problem hiding this comment.
Initially i implemented it to check for just MetaClassImpl, but after checking jackson-databind, i just replicated its exclusion.
is this correct? or should we go for targeted exclusions?
There was a problem hiding this comment.
Note calling line:
which actually checks class name: helper method name is bit misleading I agree. But yes, I think we should target to just that class, unless proven something else is needed.
There was a problem hiding this comment.
understood, I missed this code, will implement in a similar fashion.
|
Ok: I was able to make groovy test actually run -- but it does not appear to trigger cyclic dependency even without change to So not 100% sure how to trigger the problem being fixed, still. |
|
|
@Shounaks Does not fail for me if I change Could it be you are commenting out more than the new check, so that pre-existing checks for missing type or |
|
Oh wait. I can actually reproduce the issue via IDE (Eclipse) after installing Groovy plugin. It just does not fail with Maven from command-line. This is bizarre... |
|
Ok. I really wish Maven was able to reproduce (to guard against regression), but it is what it is. Will merge in for 2.17.0(-rc1). |
yeah, this is strange. Never would've guessed this can be a point of failure. 😆 |
|
Yeah more commonly it's the opposite: some problem with IDE, works as expected via Maven from shell. |
Issue: #93
Description: As per our discussion, we will be skipping serialization of Groovy related classes , i have added a sample groovy test case as well!
Reference taken from: https://github.com/FasterXML/jackson-databind/blob/db95863d6f48cb5695bf8e14af1e7af2ece8e52e/src/main/java/com/fasterxml/jackson/databind/util/BeanUtil.java#L205